-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Conditions to status #91
Conversation
* Adds an initial Condition: "Ready" with a "False" status. * Fixed operator-framework#73 * Include unit-tests Signed-off-by: Todd Short <tshort@redhat.com>
// OperatorStatus defines the observed state of Operator | ||
type OperatorStatus struct{} | ||
type OperatorStatus struct { | ||
Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen (and I haven't looked very hard) patchStrategy and patchMergeKey on conditions before. Is this something suggested in official k8s docs somewhere?
I'm wondering what the implications are? It seems like this would permit two writers to server-side apply two different types concurrently? Do we want that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these are the recommended attributes:
https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Condition
What it should mean is that you can only have a single instance of each type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also need the other tags for Conditions
that are listed in the example FooStatus
on that page? I'm not actually sure, but it's definitely worth adding them and running make generate manifests
to see what they do to the generated code and/or CRD file.
type FooStatus struct{
// Represents the observations of a foo's current state.
// Known .status.conditions.type are: "Available", "Progressing", and "Degraded"
// +patchMergeKey=type
// +patchStrategy=merge
// +listType=map
// +listMapKey=type
Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
// other fields
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does add something (x-kubernetes-list
) values. So I might as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can only have a single instance of each type.
Another follow-up/TODO/ground rule/invariant:
For every condition.Type
that we define, that type has a condition set no matter what code path is taken through the reconciler. Every single time reconcile is called, we make sure to evaluate what the value of each condition.Type
should be and we make sure it is set that way.
TL;DR:
- Reconcile is idempotent. Same input always results in the same output.
- Output always includes exactly one of each condition type.
Where input is defined as: "Operator CR (minus its status) + existing cluster state"
And output is defined as: "Operator CR status + new desired cluster state"
And now the basic unit tests are actually run... |
@joelanford I believe I've addressed all comments |
This is for: https://issues.redhat.com/browse/OLM-2854 |
controllers/operator_controller.go
Outdated
// Compare resources - ignoring status - and update if required | ||
existingOp.Status, reconciledOp.Status = operatorsv1alpha1.OperatorStatus{}, operatorsv1alpha1.OperatorStatus{} | ||
if !equality.Semantic.DeepEqual(existingOp, reconciledOp) { | ||
if updateErr := r.Update(ctx, reconciledOp); updateErr != nil { | ||
return res, utilerrors.NewAggregate([]error{reconcileErr, updateErr}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain when the controller would modify something other than the Operator CR's status? I can't imagine a scenario where we'd want to modify the resource's spec, is this to update something in the metadata (annotations, labels, etc..)? If the point of this update call is to modify something outside the spec, does it make sense to exclude it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was suggested by @joelanford; it's copied from the rukpak reconciler. My assumption is to make this a complete wrapper around the reconcile helper function My presumption would be to fill in default values if they were not included in the original resource (if we wanted to do that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(And possibly metadata)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finalizers are the only thing that comes to mind. Defaulting should be handled for us by the apiserver during admission. Not reconciliation.
This is a good question though. Perhaps we should have the code panic if it detects a change we made to non-status, non-finalizers, as that would indicate a programming mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should rukpak be updated as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelanford: something like?
existingOp.Status, reconciledOp.Status = operatorsv1alpha1.OperatorStatus{}, operatorsv1alpha1.OperatorStatus{}
existingOp.Finalizers, reconciledOp.Finalizers = []string{}, []string{}
if !equality.Semantic.DeepEqual(existingOp, reconciledOp) {
panic("spec or metadata changed by reconciler")
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would strip finalizers from the object, which we may not want to. Something like this may work.
existingOp.Status, reconciledOp.Status = operatorsv1alpha1.OperatorStatus{}, operatorsv1alpha1.OperatorStatus{}
if !equality.Semantic.DeepEqual(existingOp.Spec, reconciledOp.Spec) {
panic("spec or metadata changed by reconciler")
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? We aren't doing an r.Update()
in this case, although that might still be necessary due to updated finalizers. We can't just compare the Specs
, as that ignores other metadata
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, weirdness! r.Status().Update()
actually makes additional modifications to the structure. So, do all the comparisons first, then update the status, panic()
if required, then update the spec (should just be the finalizers).
The reconciledOp
existingOp
and compareOp
are all local variables, and k8s is unmodified until an Update()
call is made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? We aren't doing an r.Update() in this case, although that might still be necessary due to updated finalizers.
Ah sorry, I didn't notice that you changed the code between the time that I initially reviewed the PR and made that comment.
We can't just compare the Specs, as that ignores other metadata.
Controllers are allowed to edit metadata and remove finalizers, if we're saying that this controller specifically isn't allowed to make meatadata changes, this is fine.
compareOp := reconciledOp.DeepCopy() | ||
existingOp.Status, compareOp.Status = operatorsv1alpha1.OperatorStatus{}, operatorsv1alpha1.OperatorStatus{} | ||
existingOp.Finalizers, compareOp.Finalizers = []string{}, []string{} | ||
specDiffers := !equality.Semantic.DeepEqual(existingOp, compareOp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is compareOp needed? Could you not compare the specs directly?
specDiffers := !equality.Semantic.DeepEqual(existingOp, compareOp) | |
specDiffers := !equality.Semantic.DeepEqual(&existingOp.Spec, &reconciled.Spec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not comparing just specs, but specs and the metadata as well. Want to make sure no annotations or labels or etc were added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we didn't care about the metadata, then that would be correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes that we want to allow changes to the metadata field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, it's only the finalizers that can change. Any other metadata change will cause a panic()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specDiffers
could be better named.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about unexpectedFieldsChanged
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unresolved only because I'm not sure you'd see my comment. Not merge blocking though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it!
if updateFinalizers { | ||
if updateErr := r.Update(ctx, reconciledOp); updateErr != nil { | ||
return res, utilerrors.NewAggregate([]error{reconcileErr, updateErr}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than "updateFinalizers", should this be "updateMetadata"? CC @joelanford
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that there may be instances where we want to change metadata on the object we're reconciling. If we're suggesting that there is not, then we can leave it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A prior comment from @joelanford implies the only thing that the reconciler could change are the Finalizers and the Status. So those are checked explicitly to see if they changed. If anything else was updated, then there's a panic()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Admittedly, updateFinalizers
causes all of the resource (sans Status) to be updated, but if anything else were to change, it'd panic()
first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it's a good idea to only allow finalizers to be changed. As a rule, reconcilers should only change non-status, non-finalizer fields on objects they manage/own, not their primary objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @tmshort, it may be worth adding a note that we're choosing not to allow changes to the object's metadata other than its finalizers, but this is fine.
/approve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
if specDiffers { | ||
panic("spec or metadata changed by reconciler") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are probably good arguments on both sides, but did we consider moving this before the status update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it some thought. But I wasn't sure. Having the Status
update at least gives an idea of what error may have led to this condition. But this is "something that shouldn't happen"™, and should never be seen after release.
Status: metav1.ConditionFalse, | ||
Reason: operatorsv1alpha1.ReasonNotImplemented, | ||
Message: "The Reconcile operation is not implemented", | ||
ObservedGeneration: op.GetGeneration(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this reminds me of another way to test for making sure we update all conditions. Unit tests of the Reconcile
method should make sure that all condition types have condition objects AND all of those objects have observedGeneration that matches the metadata.generation of the Operator that was reconciled.
It("has a Condition created", func() { | ||
getOperator := &operatorsv1alpha1.Operator{} | ||
|
||
err = k8sClient.Get(ctx, client.ObjectKey{ | ||
Name: opName, | ||
}, getOperator) | ||
Expect(err).To(Not(HaveOccurred())) | ||
|
||
// There should always be a "Ready" condition, regardless of Status. | ||
conds := getOperator.Status.Conditions | ||
Expect(conds).To(Not(BeEmpty())) | ||
Expect(conds).To(ContainElement(HaveField("Type", operatorsv1alpha1.TypeReady))) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, but perhaps some good future proofing to do while things aren't already baked. Could we declare a slice of all of our condition types in the same place the we define them, and then in this test iterate that slice to make sure that all of them exist and have the generation of the opName
operator?
// There should always be a "Ready" condition, regardless of Status. | ||
conds := getOperator.Status.Conditions | ||
Expect(conds).To(Not(BeEmpty())) | ||
Expect(conds).To(ContainElement(HaveField("Type", operatorsv1alpha1.TypeReady))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also test for the specific Status and Reason we expect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to not bother with that. As I wanted to minimize changes (vs. additions) to the tests as code is added.
Thank you all. I'll do a follow up to address some of minor issues/nits. |
Make sure each condition is defined (as best we can) Enhancements to operator-framework#91 Signed-off-by: Todd Short <tshort@redhat.com>
* Add additional unit-tests for Conditions Make sure each condition is defined (as best we can) Enhancements to #91 Signed-off-by: Todd Short <tshort@redhat.com>
Signed-off-by: Todd Short tshort@redhat.com
For: https://issues.redhat.com/browse/OLM-2854